fix: adding data specific p-value filters #788
Conversation
| @@ -37,6 +37,10 @@ def __init__( | |||
| finngen_susie_finemapping_cs_summary_files=finngen_susie_finemapping_cs_summary_files, | |||
| ) | |||
|
|
|||
| finngen_finemapping_df = finngen_finemapping_df.validate_lead_pvalue( | |||
There was a problem hiding this comment.
Minor stylistic comment, and it is absolutely my preference, but I like to use as few variables as possible:
(
# Reading Finngen finemapped dataset and convert it to study locus:
FinnGenFinemapping.from_finngen_susie_finemapping(
spark=session.spark,
finngen_susie_finemapping_snp_files=finngen_susie_finemapping_snp_files,
finngen_susie_finemapping_cs_summary_files=finngen_susie_finemapping_cs_summary_files,
)
# Flagging sub-significnat loci:
.validate_lead_pvalue(
pvalue_cutoff=FinngenFinemappingConfig().finngen_finemapping_lead_pvalue_threshold
)
# Write the output.
.df.write.mode(session.write_mode).parquet(
finngen_finemapping_out
)
)There was a problem hiding this comment.
Please carefully check, I removed the variables but not sure
| @@ -169,6 +175,7 @@ class FinngenFinemappingConfig(StepConfig): | |||
| _target_: str = ( | |||
| "gentropy.finngen_finemapping_ingestion.FinnGenFinemappingIngestionStep" | |||
| ) | |||
| finngen_finemapping_lead_pvalue_threshold: float = 1e-5 | |||
There was a problem hiding this comment.
There's one thing I'm not sure about. You have added finngen_finemapping_lead_pvalue_threshold to the relevant config, and refer to as pvalue_cutoff=FinngenFinemappingConfig().finngen_finemapping_lead_pvalue_threshold in the step. However, finngen_finemapping_lead_pvalue_threshold it not an argument for FinnGenFinemappingIngestionStep. Is it OK? Would all parameters in the config passed to the step? Would that cause any problem? @project-defiant , what do you think?
There was a problem hiding this comment.
OK, double-checked with @project-defiant and @d0choa and all parameters in the setepConfig classes needs to be parameters in the init function of the step.
src/gentropy/eqtl_catalogue.py
Outdated
| EqtlCatalogueFinemapping.from_susie_results(processed_susie_df) | ||
| # Flagging sub-significnat loci: | ||
| .validate_lead_pvalue( | ||
| pvalue_cutoff=EqtlCatalogueConfig().eqtl_lead_pvalue_threshold |
There was a problem hiding this comment.
Same comment: EqtlCatalogueConfig().eqtl_lead_pvalue_threshold has to be passed as parameter of the step.
| ) | ||
| # Flagging sub-significnat loci: | ||
| .validate_lead_pvalue( | ||
| pvalue_cutoff=FinngenFinemappingConfig().finngen_finemapping_lead_pvalue_threshold |
There was a problem hiding this comment.
Same comment: FinngenFinemappingConfig().finngen_finemapping_lead_pvalue_threshold has to be an init parameter of the step.
| .filter_credible_set(credible_interval=CredibleInterval.IS99) | ||
| # Flagging sub-significnat loci: | ||
| .validate_lead_pvalue( | ||
| pvalue_cutoff=WindowBasedClumpingStepConfig().gwas_significance |
Fixed. Please check |
|
There was a problem hiding this comment.
This PR as it is now, makes it impossible to parametrise the p-value threshold of the PICS step. IF other steps are called as a stand-alone command, users can provide the custom p-value threshold. However this is not true for PICS step. The applied cutoff is defined by config (WindowBasedClumpingStepConfig().gwas_significance), which value cannot be overridden.
I think this might be fine for now, but we should follow the pattern of other steps where all parameters inside the steps are customisable.
✨ Context
Data specific lead p-value CS filters.
🛠 What does this PR implement
🙈 Missing
🚦 Before submitting
devbranch?make test)?poetry run pre-commit run --all-files)?